Skip to content

[SPARK-8920] [MLlib] Add @since tags to mllib.linalg#7729

Closed
sabhyankar wants to merge 6 commits into
apache:masterfrom
sabhyankar:branch_8920
Closed

[SPARK-8920] [MLlib] Add @since tags to mllib.linalg#7729
sabhyankar wants to merge 6 commits into
apache:masterfrom
sabhyankar:branch_8920

Conversation

@sabhyankar

Copy link
Copy Markdown

No description provided.

@mengxr

mengxr commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

ok to test

@mengxr

mengxr commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

@MechCoder Could you help review this PR? Thanks!

@SparkQA

SparkQA commented Jul 28, 2015

Copy link
Copy Markdown

Test build #38755 has finished for PR 7729 at commit 2e5ebd6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that a sealed trait should be tagged?

@sabhyankar

Copy link
Copy Markdown
Author

@MechCoder Thanks for pointing that out. I agree that the tag might not be necessary for the sealed trait. Let me know if you come across anything else.

@SparkQA

SparkQA commented Aug 1, 2015

Copy link
Copy Markdown

Test build #39377 has finished for PR 7729 at commit 3be09e9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that these methods were added to the DenseMatrix object in 1.3.0 right?

@MechCoder

Copy link
Copy Markdown
Contributor

Thanks for the PR. I'm also not sure if tags should be added for hashcode, equals etc since the user is not going to access these features through the documentation.

@sabhyankar

Copy link
Copy Markdown
Author

@MechCoder thank you for the very detailed review! I have made most of the changes that you had pointed out except for the following:

  • I left the tags on the alternative constructor for the public classes as I thought it would be useful to know when it was introduced.
  • I left the tag in the DistributedMatrix as it is not a sealed trait and would be accessible.

In addition to the above:

  • I removed the tags from the hashCode and equals methods based on your comment. The only exception is the toString method in the DenseVector and SparseVector classes.

Let me know if you spot something else. Thanks again.

@SparkQA

SparkQA commented Aug 4, 2015

Copy link
Copy Markdown

Test build #39634 has finished for PR 7729 at commit 3012864.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MechCoder

Copy link
Copy Markdown
Contributor

I left the tags on the alternative constructor for the public classes.

Then could you make the tags availble for all the alternative constructors. eg. DenseMatrix

@sabhyankar

Copy link
Copy Markdown
Author

@MechCoder I couldnt find the untagged alternate constructor in DenseMatrix. Can you point me to where you see it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MechCoder I sincerely apologize for my ignorance here! I am a little new to Scala and am not sure what I am missing here. I will be happy to fix it if you can provide some clarification. I have fixed the other errors that you identified below and will update the PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh never mind, I could notice that from the diff view :( . Looks all right to me.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) Thanks!

@MechCoder

Copy link
Copy Markdown
Contributor

That's it apart from those couple of comments.

@mengxr

mengxr commented Aug 11, 2015

Copy link
Copy Markdown
Contributor

We don't need to add since tags for methods inherited from Java Object like toString, hashCode, and equals. @sabhyankar Could you address @MechCoder 's comments? We should merge this before the 1.5 RC release.

@sabhyankar

Copy link
Copy Markdown
Author

I am going to take a look at this and update the PR by tomorrow.

@mengxr

mengxr commented Aug 14, 2015

Copy link
Copy Markdown
Contributor

test this please

@mengxr

mengxr commented Aug 14, 2015

Copy link
Copy Markdown
Contributor

@MechCoder Could you make another pass? Thanks!

@sabhyankar

Copy link
Copy Markdown
Author

I removed the tags from two of the toString method and confirmed left the tags in place for the alternate constructors as mechcoder mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants